Added #2353: Add ability to tie locations to companies - 2023 edition #12577

Toreg87 · 2023-02-28T14:11:59Z

Description

Locations are the last big part of the application that can't be tied to companies.
This can be a problem with FullMultipleCompanySupport, because you can't restrict the visibility of locations to the company of the users.

The first commit wires everything up so that locations have a company_id and are properly scoped with FullMultipleCompanySupport.

The second commit introduce a setting to preserve backward compatibility with the existing behaviour.

This is an updated version of PR #10705. It is rebased to the current develop branch and a few bugs are fixed. I'm quite happy with the backward compatibility setting, so that erveryone can select the behaviour they want.

Fixes #2353

Type of change

Please delete options that are not relevant.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration

I checked that the correct locations are shown as SuperUser and scoped user with FullMultipleCompanySupport.
The commits are tested with and without the backward compatibility setting.

Checklist:

I have read the Contributing documentation available here: https://snipe-it.readme.io/docs/contributing-overview
I have formatted this PR according to the project guidelines: https://snipe-it.readme.io/docs/contributing-overview#pull-request-guidelines
My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

what-the-diff · 2023-02-28T14:12:56Z

Added company_id to locations table
Add location -> company relationship in Location model
Scope Locations by Company if setting is enabled (Api/LocationsController, LocationsController)
Display the new column on the index page and add it as a searchable field for datatables (LocationPresenter)
Add scope_locations_fmcs setting to settings table
Update translations for new setting
Scope locations by company in location index view and api call (index)
Show company name on location print page, if available
Show manager and parent as links on the edit modal of a location

Toreg87 · 2023-03-01T09:49:01Z

I found another bug in not using the CompanyableTrait in the location model. This complicates the backward compatibility a bit, but does now work correctly. I pushed the changes, this is now fixed.

hogemo · 2023-05-24T13:01:05Z

Will this be available in the next version of Snipe IT? I really look forward for this change to be applied :)

snipe · 2023-05-24T20:09:31Z

@paegelow not in the next version, but it's not off the table for merging. Again, this requires extensive testing as it can introduce security issues, and we're currently working on closing out the v6.x series to make way for v7.

Toreg87 · 2023-06-01T08:09:06Z

@snipe I can help to test these changes and make adjustments if this is desired. Looking at Issue #2353 I think there is demand for this feature.

hogemo · 2023-08-10T08:28:56Z

@paegelow not in the next version, but it's not off the table for merging. Again, this requires extensive testing as it can introduce security issues, and we're currently working on closing out the v6.x series to make way for v7.

Hi again!

Could you provide a tentative timeline or any updates on when this might be reviewed for merging, especially considering the transition to v7? I understand if it's challenging to provide an exact date, but any insights would be greatly appreciated.

Thank you!

hogemo · 2023-08-22T11:14:54Z

@Toreg87 Hi! Perhaps a stupid question, but how would i implement this in the most efficient way? Manually editing my files, and copy pasteing the changes?

Or, some git command? (i've done that before with not so much luck, so i ended up copy/pasteing, which also worked, but now with 17 files involved it seems a bit tedious..)

marcusmoore

Thanks for your work on this @Toreg87 😄

I have some minor requests.

Also, I want to figure out if we can move away from using the Location's constructor for adding the scope as well. I'll put some thought into that.

Would you be ok with me pushing tests to your branch?

marcusmoore · 2023-08-22T22:38:29Z

database/migrations/2023_02_27_092130_add_scope_locations_setting.php

+     */
+    public function down()
+    {
+        Schema::table('locations', function (Blueprint $table) {


This should reference the settings table.

marcusmoore · 2023-08-22T22:40:04Z

database/migrations/2023_02_27_092130_add_scope_locations_setting.php

+    public function up()
+    {
+        Schema::table('settings', function (Blueprint $table) {
+            $table->boolean('scope_locations_fmcs')->default('0');


This is a nitpick but can you group this column with full_multiple_companies_support please?

Suggested change

$table->boolean('scope_locations_fmcs')->default('0');

$table->boolean('scope_locations_fmcs')->default('0')->after('full_multiple_companies_support');

Ok, changed.

marcusmoore · 2023-08-22T22:45:56Z

resources/views/settings/general.blade.php

+                                {{ trans('admin/settings/general.scope_locations_fmcs_support_help_text') }}
+                            </p>
+                        </div>
+                    </div>


Can you update the mark up so the label is next to the input instead of below it ?

I think you need to wrap it in a label.form-control

marcusmoore · 2023-08-22T22:54:04Z

app/Http/Controllers/SettingsController.php

+        // Backward compatibility for locations makes no sense without FullMultipleCompanySupport
+        if (!$setting->full_multiple_companies_support) {
+            $setting->scope_locations_fmcs = '0';
+        }


This makes sense but might confuse some users. I wonder if there is a way to concisely express this on the settings page? 🤔 // @snipe

I think this can be handled in the view with some javascript. If this is a problem I can think about it some more, but my experience with this is quite limited.

marcusmoore · 2023-08-22T23:07:39Z

app/Models/Location.php

+    */
+    public function company()
+    {
+        return $this->belongsTo('\App\Models\Company', 'company_id');


Just to match the style of surrounding methods:

Suggested change

return $this->belongsTo('\App\Models\Company', 'company_id');

return $this->belongsTo(\App\Models\Company::class, 'company_id');

marcusmoore · 2023-08-22T23:30:34Z

app/Http/Controllers/Api/LocationsController.php

+        // Only scope locations if the setting is enabled
+        if (Setting::getSettings()->scope_locations_fmcs) {
+            $locations = Company::scopeCompanyables($locations);
+        }


Should this be wrapped in the following like we have in other places?

// Only scope the location if the setting is enabled if (Setting::getSettings()->scope_locations_fmcs) { $location->company_id = Company::getIdForCurrentUser($request->input('company_id')); } else { $location->company_id = $request->input('company_id'); }

This is in line with other scoped controllers and not about the company_id or am I misunderstanding something?

marcusmoore · 2023-08-22T23:40:55Z

app/Models/Location.php

@@ -17,6 +18,16 @@

 class Location extends SnipeModel
 {
+    function __construct() {


I get the intent but I'd like to see if there is another way to approach this. Can this be handled in the booted method?

Toreg87 · 2023-08-23T15:34:52Z

@marcusmoore I'm currently on vacation until september. After that I will look at your requested changes. The constructor looks awfull, but this was the only way I could find to implement this. Suggestions for better alternatives are highly welcome. You can take the branch and push tests to it, no problem.

marcusmoore · 2023-08-29T00:14:31Z

Note for myself/whoever is interested: The addition of the __construct method breaks the location factory so we can't seed or run tests in the current state.

I'm still working through this.

Toreg87 · 2023-09-13T09:14:11Z

@marcusmoore I rebased my branch and fixed or commented your requested changes. I found another slightly less ugly way for the backward compatibility without the constructor. What do you think about it? I left the initial approach in the branch for discussion and can delete it later, if desired.

marcusmoore · 2023-09-13T18:48:49Z

@Toreg87 thanks for updating and addressing the comments. I haven't had a chance to dive back into this but will as soon as I can.

hogemo · 2023-11-07T14:37:47Z

Is there any update on this case? I have alot of users that are eager to try this :)

marcusmoore · 2023-11-09T01:46:37Z

@paegelow this hasn't fallen off of my radar but unfortunately I haven't had a chance to get back to it yet.

dcdamien · 2024-01-04T11:13:42Z

@marcusmoore is there something we can do to make this feature merged? This is really important for us.

marcusmoore · 2024-01-08T19:25:32Z

@dcdamien thanks for your interest in this feature. It is still on my radar but I haven't had a chance to focus on it again but hope to in the near future. I have follow up on @Toreg87's changes before I can say if there other things to address before it is merged.

I've moved this higher up on my priority list and hope to dive back in soon.

Archi-web3 · 2024-02-08T11:20:52Z

@marcusmoore Thank you very much Marcus for working on this feature, which is very important for many people, and I'll be keeping a close eye on progress. It's the only thing that has so far restricted us from using Snipe IT (I work for an international organization present in over 25 countries that would be interested in using Snipe IT). Is there anything we can do to help you move this forward? Provide a developer? or something. I know this might be hard to estimate, but do you have any idea about the timing of seeing this feature in a new version?

AdrianOsica · 2024-02-13T20:39:30Z

Hi, @marcusmoore What is your workload like? Do you already know the expected time for completion of this merge?

dcdamien · 2024-09-23T19:37:34Z

@marcusmoore it seems your radar is rather long range one. In a little over 2 months, it will be appropirate to create new merge request, 2025 edition. Any chances to review it before end of the year?

snipe · 2024-09-23T21:49:07Z

@dcdamien being a jerk to the devs isn’t going to get this merged faster. This is a dangerous PR as it inherently changes previous behavior that people have come to expect and rely on and there are merge conflicts and tests are failing. More flies with honey than vinagre and all that.

dcdamien · 2024-09-25T13:13:47Z

@dcdamien being a jerk to the devs isn’t going to get this merged faster.

@snipe Well, note that my comment, unlike yours, does not offend anyone. I think you took it too seriously. Anyway, I think we won't focus on that, let's get to the specifics.

This is a dangerous PR as it inherently changes previous behavior that people have come to expect and rely

Doesn't the MR address this issue by providing backward compatibility? Can you please explain what exacly is dangerous?
IMO the current behaviour is more dangerous as locations are leaking between organizations when the Full Multiple Companies Support is enabled.

on and there are merge conflicts

This is nominal that stale MR begins to conflict with the upstream branch, right?

and tests are failing.

This MR is that old that I even can't check what's wrong with CI.

snipe · 2024-09-25T16:01:07Z

Well, note that my comment, unlike yours, does not offend anyone. I think you took it too seriously. Anyway, I think we won't focus on that, let's get to the specifics.

If I didn't think your comment was offensive, I wouldn't have said anything.

I think you took it too seriously.

I do take these things seriously, especially when people are being rude to my developers. The snide remark isn't useful.

Doesn't the MR address this issue by providing backward compatibility?

Yes, which adds additional test points and cyclomatic complexity, and potential complications with data down the line.

This is nominal that stale MR begins to conflict with the upstream branch, right?

If the OP can resolve those conflicts, we can take another look. A lot of things have changed since the original PR was opened, and guessing intent can be difficult with so many conflicts.

Toreg87 · 2024-10-02T07:25:38Z

I try to find time to rebase this in the next couple of weeks.

dreamer2038 · 2024-10-07T09:06:58Z

I try to find time to rebase this in the next couple of weeks.

thank you for your hard work

protocol6v · 2024-10-18T17:58:14Z

This feature is something that is stopping my company from deploying this to our end users. Its a critical privacy concern for us as an upstream company that "end users", who we need at various other 3rd party company locations to manage their assets, could view our entire locations list across all companies.

Would really really like to see this implemented soon.

Locations are the last big part of the application that can't be tied to companies. This can be a problem with FullMultipleCompanySupport, because you can't restrict the visibility of locations to the company of the users. In order to change this, add a company_id to the locations table and wire everything up in the views and controllers. Aditionally add a new formatter to filter the locations to a specific company, like it is done for assets. Locations are properly scoped to the users company if FullMultipleCompanySupport is enabled. If a parent location of a location has a different company than the user, the location does not show up.

Now that locations have a company_id they get restricted to the users company with FullMultipleCompanySupport. This breaks backward compatibility, because before everyone can handle locations without restrictions. Add a setting right below FullMultipleCompanySupport so that everyone can switch to the desired behaviour. The default is off and the existing behaviour is preserved.

Instead of using a constructor, add a special check in the boot-method for locations. This seems to fit better in the system and does hopefully not break the existing tests. Signed-off-by: Tobias Regnery <[email protected]>

Toreg87 · 2024-11-04T09:44:31Z

@snipe I rebased my branch to the current version, can someone please have a look and review this?

One pain point is the parent-child-relationship of locations. With scoped locations and a parent with different company the relationship obviously isn't visible for scoped users. I don't know if this is a problem. For me it isn't, if I can scope my locations I don't need a parent location. But maybe others have different needs. Maybe we could introduce a bulk option and error out if there are different companies involved.

But before I invest more time in this I think it would be good to get an agreement that this feature is useful und acceptable.

marcusmoore · 2024-11-07T17:54:21Z

@Toreg87

But before I invest more time in this I think it would be good to get an agreement that this feature is useful und acceptable.

I completely agree and really appreciate your effort. Please give us a time to discuss this internally to make sure it is the right approach for the majority of users. I'd love to see this merged in but we went to be extra cautious it doesn't cause issues down the line.

dcdamien · 2024-11-27T12:50:07Z

@snipe @marcusmoore please comment on @Toreg87 work.

swift2512 · 2024-12-17T13:35:50Z

Aren't you overcomplicating everything? :) Departments are attached to companies, so why locations can't use same logic? This would be first working step in right direction. Can't imagine how binding location to company could impact users who don't use multiple companies. (And if they use multi company support, they probably hate current location system.) Multiple Companies using same location sounds odd, but even in this case nobody prevents you from creating another location with exact same name (ID will be different anyway). People from Company A can't see people, assets, departments in Company B so what changes if we add locations to that pool? I believe, nothing.

Currently, like others stated, locations are leaking to every single company (when Multi-Company is enabled) and anyone from any company can edit/delete those locations if have right permissions. (Actually, such behaviour is a GDPR breach.)

Second step - tool that displays a location tree for parent location and it's children. This tree could be put in the separate tab on 'location view' and it could be displayed like this:

PARENT (1)
↳Child (1.1)
  ↳Child (1.1.1)
  ↳Child (1.1.2)
    ↳Child (1.1.2.1)
  ↳Child (1.2)
  ↳Child (1.3

Third step - option to tie user to location where user or admin can't see users, items and locations higher then their assigned parent location (It's called 'scoped location'?). This alone removes the need to enable multi-company in the settings, because you can have different companies as different location trees. You would enable Multi-Company support if you really need people from different companies to use same Snipe-IT install.

swift2512 · 2024-12-17T13:54:20Z

One pain point is the parent-child-relationship of locations. With scoped locations and a parent with different company the relationship obviously isn't visible for scoped users. I don't know if this is a problem. For me it isn't, if I can scope my locations I don't need a parent location. But maybe others have different needs. Maybe we could introduce a bulk option and error out if there are different companies involved.

@Toreg87 why would PARENT be in different company? Company is a part of another company? (I believe that organizations of such scale use proprietary paid solutions.)

Toreg87 · 2024-12-20T07:57:58Z

Well maybe, but different comapnies have different needs. I wanted to make sure that this is a general solution for everyone. We can discuss the implementation when there is consensus with the devs that this feature can be merged.

snipe · 2025-01-17T17:27:56Z

@Toreg87 why would PARENT be in different company?

@swift2512 because sometimes people do goofy things. We either have to prevent them from doing goofy things, or handle the weird scenarios that could come up if they should choose to do that.

@Toreg87 - I just handled a few merge conflicts that have since come up. (Thanks again for sticking with this.)

If folks have a an asset that belongs to Company A, and is in Location A, but Location A belongs to Company B, do we have a way of preventing that change, or warning folks? I could definitely see that getting super confusing.

Toreg87 · 2025-01-23T08:42:20Z

@snipe I thought about this a litte bit and came up with 3 problematic cases:

Initial change of settings
If an admin changes to fmcs without backward compatibility there could be some kind of helper that checks every location for inconsistencies (assigned assets, users, etc. with different company or locations with different companies in parent or childs). Depending on the size of the database, this could be time consuming so there must be some kind of UI to display the progress.
Setting is enabled and user changes company of an asset, user, etc.
In this case a check must be made that location_id is null (and in case of assets rtd_location and assigned_to if checkout_type = location). If 1. is in place there couldn't be preexisting problems.
Setting is enabled and user changes company of a location
Here we need to check that no asset, user, etc. is assigned to this location. If 1. is in place there couldn't be preexisting problems. Another check must be made for parent or childs with different companies. To change the company of a location in a hierarchy we either have to say that this isn't possible or we need some sort of bulk edit for locations.

I think 2. and 3. is quite easy to implement, 1. seems a little bit harder. Do you think this is reasonable or are there other cases I overlooked?

snipe · 2025-01-23T09:52:14Z

@Toreg87 No, definitely - these are the scenarios I have been so hesitant about. Moving forward is easy, it's all the existing data which could have been mixed that's always concerned me. (We've seen a lot of reeeeally weird choices made by our users over the years lol)

A child/parent being in a different company is one I can definitely see causing real problems with existing data, since we don't have the concept of parent/child companies, and some folks have a parent company and then smaller companies underneath.

Number 1 is very much the hardest part to handle, IMHO, for the reasons you state. We'd effectively have to walk through every item and person to make sure everything matched, every single time that setting is turned on.

swift2512 · 2025-01-23T12:46:11Z

Why not handle this like any proprietary software - do the switch and users will follow. :D Snipe-IT is in it's own league with every single competitor being only paid option. Enabling the new way of dealing with locations should have red banner "there's no return after this" and users decide do they want to switch or not. I believe, part would switch to new model because of all these leaking locations (if you are dealing with outside companies, this might be seen as GDPR breach in Europe.).
Other people (including myself) may stick to old way because they are using Multi-Company feature to prevent certain group of people (different departments) from managing items they shouldn't. With current architecture, there's no Company hierarchy, so you either can only see items from Company you are attached to or nothing at all, if you are not attached to any company, and there are no other items/people without company. Only super-user can see all items from all 'companies' in that particular location, but I don't think it's wise to give such rights to regular asset managers.

Food for thought
Users and locations can be assigned to multiple companies in the same way people are assigned to multiple user-groups. Or it's still a 'single company only' for users, but in Permission table (or user-group) you can mark companies that user has access to. (It would be like a some sort of company override)
This way you still can decide who sees what, but at the same time people (asset managers without super-user rights) from different companies can access same locations and people. Also, it becomes possible to have regular users with limited set of permissions who can manage items and people from different 'Companies'.

snipe · 2025-01-23T12:48:17Z

Food for thought

This has been discussed many times over the years, and is still on the table, it's just very complicated to build and test while making sure nothing slips through the cracks.

Before activating scoped location all locations and their related objects will be checked. If there are locations with different companies than the related objects error out. Because this operation is quite slow, bail out on the first inconsistent entry. There is a new artisan command introduced that checks every location. Depending on the size of the database, this will take very long. Signed-off-by: Tobias Regnery <[email protected]>

Toreg87 · 2025-01-23T14:34:41Z

I tried implementing number 1 and it isn't as ugly as I thought. It is obviously quite slow but you don't have to run this after the initial problems are solved. This need some further polishing but I think you should get the idea. What do you think?

There is a new validator introduced that checks on object update (assets, users, etc.) if the company matches the locations company. In case of the creation of a new location it must be checked that the parent matches the own company. On updating a location a check for every related object must be made to see if the company matches the location. Signed-off-by: Tobias Regnery <[email protected]>

Toreg87 · 2025-01-24T10:21:19Z

So the validation for the other cases is now also in place. This is only lightly tested because my database obviously don't has locations with company_ids. It also needs polishing with proper error messages and the like but it handles every case I can currently think of. @snipe how can we proceed further?

Toreg87 requested a review from snipe as a code owner February 28, 2023 14:12

probot-autolabeler bot added api backend frontend labels Feb 28, 2023

Toreg87 mentioned this pull request Feb 28, 2023

Added #2353: Add ability to tie locations to companies #10705

Closed

11 tasks

Toreg87 force-pushed the feature/locations_with_companies branch from 77eb86f to 6b0a82c Compare March 1, 2023 09:45

probot-autolabeler bot added the translations label May 24, 2023

snipe requested review from uberbrady, marcusmoore and spencerrlongg August 22, 2023 11:30

marcusmoore self-assigned this Aug 22, 2023

marcusmoore requested changes Aug 22, 2023

View reviewed changes

Toreg87 force-pushed the feature/locations_with_companies branch from c3e1f8e to abae865 Compare September 13, 2023 09:08

snipe mentioned this pull request Apr 8, 2024

[Feature Request]: locations bound to companies #14570

Closed

Toreg87 added 3 commits November 4, 2024 10:03

Another slightly less ugly way for backward compatibility

651d1c7

Instead of using a constructor, add a special check in the boot-method for locations. This seems to fit better in the system and does hopefully not break the existing tests. Signed-off-by: Tobias Regnery <[email protected]>

Toreg87 force-pushed the feature/locations_with_companies branch from abae865 to 651d1c7 Compare November 4, 2024 09:35

snipe mentioned this pull request Jan 17, 2025

Locations: Add field company in location parameters #15971

Open

Merge branch 'develop' into feature/locations_with_companies

b6f05bf

probot-autolabeler bot added the cli / artisan label Jan 23, 2025

	$table->boolean('scope_locations_fmcs')->default('0');
	$table->boolean('scope_locations_fmcs')->default('0')->after('full_multiple_companies_support');

	return $this->belongsTo('\App\Models\Company', 'company_id');
	return $this->belongsTo(\App\Models\Company::class, 'company_id');

Added #2353: Add ability to tie locations to companies - 2023 edition #12577

Are you sure you want to change the base?

Added #2353: Add ability to tie locations to companies - 2023 edition #12577

Conversation

Toreg87 commented Feb 28, 2023

Description

Type of change

How Has This Been Tested?

Checklist:

what-the-diff bot commented Feb 28, 2023

Toreg87 commented Mar 1, 2023

hogemo commented May 24, 2023 • edited Loading

snipe commented May 24, 2023

Toreg87 commented Jun 1, 2023

hogemo commented Aug 10, 2023

hogemo commented Aug 22, 2023

marcusmoore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toreg87 commented Aug 23, 2023

marcusmoore commented Aug 29, 2023

Toreg87 commented Sep 13, 2023

marcusmoore commented Sep 13, 2023

hogemo commented Nov 7, 2023

marcusmoore commented Nov 9, 2023

dcdamien commented Jan 4, 2024

marcusmoore commented Jan 8, 2024

Archi-web3 commented Feb 8, 2024 • edited Loading

AdrianOsica commented Feb 13, 2024

dcdamien commented Sep 23, 2024 • edited Loading

snipe commented Sep 23, 2024 • edited Loading

dcdamien commented Sep 25, 2024

snipe commented Sep 25, 2024 • edited Loading

Toreg87 commented Oct 2, 2024

dreamer2038 commented Oct 7, 2024

protocol6v commented Oct 18, 2024

Toreg87 commented Nov 4, 2024

marcusmoore commented Nov 7, 2024

dcdamien commented Nov 27, 2024

swift2512 commented Dec 17, 2024

swift2512 commented Dec 17, 2024 • edited by snipe Loading

Toreg87 commented Dec 20, 2024

snipe commented Jan 17, 2025

Toreg87 commented Jan 23, 2025

snipe commented Jan 23, 2025

swift2512 commented Jan 23, 2025 • edited Loading

snipe commented Jan 23, 2025

Toreg87 commented Jan 23, 2025

Toreg87 commented Jan 24, 2025

hogemo commented May 24, 2023 •

edited

Loading

Archi-web3 commented Feb 8, 2024 •

edited

Loading

dcdamien commented Sep 23, 2024 •

edited

Loading

snipe commented Sep 23, 2024 •

edited

Loading

snipe commented Sep 25, 2024 •

edited

Loading

swift2512 commented Dec 17, 2024 •

edited by snipe

Loading

swift2512 commented Jan 23, 2025 •

edited

Loading